Extend CacheRuntime phase 2.5: transform Cache Runtime Spec into pods spec#5888
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the CacheEngine component transformation and status update logic to be more modular and efficient. It introduces lightweight status tracking using ComponentIdentity and consolidates volume and pod template transformations into shared helper functions. Additionally, it replaces hardcoded labels with constants and significantly expands test coverage. Feedback identifies a bug in worker affinity construction where existing node affinity preferences are overwritten, and a potential issue with duplicate volume names in pod specs due to incomplete tracking of previously added volumes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5888 +/- ##
==========================================
+ Coverage 62.59% 63.35% +0.76%
==========================================
Files 480 481 +1
Lines 32801 33070 +269
==========================================
+ Hits 20533 20953 +420
+ Misses 10633 10442 -191
- Partials 1635 1675 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements the second phase (2.5) of CacheRuntime spec→pod transformation for master, worker and client components. It introduces dedicated transformation helpers (initComponentValue, transformComponentPodTemplate, transformVolumes, buildWorkerAffinity), splits the heavy transform() from a lightweight getRuntimeStatusValue() used by Sync/Setup for status-only updates (addressing #5877), and refactors ComponentManager.ConstructComponentStatus/GetNodeAffinity to take a minimal ComponentIdentity plus a new CacheRuntimeStatusValue/ComponentStatusInfo types instead of the full CacheRuntimeComponentValue. It also factors out hard-coded label keys (fluid.io/dataset[-placement], kubernetes.io/hostname) into common constants.
Changes:
- Refactor cache runtime transform into reusable helpers for pod template, volumes, and worker affinity, with deep-copying of the runtime class template.
- Introduce lightweight
CacheRuntimeStatusValue/ComponentStatusInfo/ComponentIdentitytypes and switch Sync/Setup status updates to them. - Replace string-literal label keys with
common.LabelAnnotationDataset[Placement]/common.K8sNodeNameLabelKeyin jindo, ctrl/affinity, and cache transforms.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/common/cacheruntime.go | Adds CacheRuntimeStatusValue, ComponentIdentity, ComponentStatusInfo, MatchLabels field; drops RuntimeIdentity from CacheRuntimeValue. |
| pkg/common/label.go | Adds LabelAnnotationDatasetPlacement constant. |
| pkg/ctrl/affinity.go, pkg/ctrl/affinity_test.go | Replace hardcoded label/topology strings with common constants. |
| pkg/ddc/jindo/worker.go, worker_test.go | Same constants migration for Jindo. |
| pkg/ddc/cache/component/component_manager.go | Interface changes: status/affinity now take ComponentIdentity. |
| pkg/ddc/cache/component/statefulset_manager.go | Switch status/affinity functions to identity argument. |
| pkg/ddc/cache/component/daemonset_manager.go | Same, plus consumes component.MatchLabels for selector. |
| pkg/ddc/cache/component/component_test.go | Updates tests for new identity-based API. |
| pkg/ddc/cache/engine/transform.go | Splits transform; adds getRuntimeStatusValue; removes addCommonConfigForComponent. |
| pkg/ddc/cache/engine/transform_common.go | New: initComponentValue and transformComponentPodTemplate (image, env, nodeSelector, tolerations, etc.). |
| pkg/ddc/cache/engine/transform_master.go / _client.go / _worker.go | Re-implemented using new helpers; worker adds in-engine affinity & MatchLabels. |
| pkg/ddc/cache/engine/transform_volumes.go | Consolidated volume transform: runtime config, extra CMs, runtime spec volumes, encrypt options. |
| pkg/ddc/cache/engine/transform_*_test.go, transform_test.go, sync_test.go, setup_test.go, component_setup_test.go | New/updated unit tests. |
| pkg/ddc/cache/engine/sync.go | Use lightweight getRuntimeStatusValue for Sync. |
| pkg/ddc/cache/engine/setup.go | Use getRuntimeStatusValue before CheckAndUpdateRuntimeStatus. |
| pkg/ddc/cache/engine/status.go, status_test.go | Status setters use ComponentStatusInfo. |
| pkg/ddc/cache/engine/master.go, worker.go, client.go | Build ComponentIdentity from component value before status query. |
| test/gha-e2e/curvine/cacheruntime.yaml | Removes unused client volumeMounts/volumes example block. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: xliuqq <xlzq1992@gmail.com> fix affinity
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/ddc/cache/engine/transform_volumes.go:204
- transformRuntimeSpecVolumes appends all runtime-provided volumeMounts directly to containers[0].VolumeMounts without dedup/override. If the runtimeClass template already defines a mount with the same Name or MountPath, this can produce duplicate mounts and an invalid PodSpec. Prefer using utils.AppendOrOverrideVolumeMounts (or similar dedup logic) when applying runtime Spec volumeMounts.
// First pass: identify which volumes are referenced by volumeMounts and add volumeMounts to container
referencedVolumeMap := make(map[string]bool)
for _, volumeMount := range volumeMounts {
referencedVolumeMap[volumeMount.Name] = true
podSpec.Containers[0].VolumeMounts = append(
podSpec.Containers[0].VolumeMounts, volumeMount,
)
}
| func (e *CacheEngine) applyRuntimeConfigVolume(podSpec *corev1.PodSpec, commonConfig *CacheRuntimeComponentCommonConfig) { | ||
| if commonConfig == nil || commonConfig.RuntimeConfigs.RuntimeConfigVolume.Name == "" { | ||
| return | ||
| } | ||
|
|
||
| // Add runtime config volume | ||
| podSpec.Volumes = append( | ||
| podSpec.Volumes, | ||
| commonConfig.RuntimeConfigs.RuntimeConfigVolume, | ||
| ) | ||
|
|
||
| // Add runtime config volume mount to init container if exists | ||
| if len(podSpec.InitContainers) > 0 { | ||
| podSpec.InitContainers[0].VolumeMounts = append( | ||
| podSpec.InitContainers[0].VolumeMounts, | ||
| commonConfig.RuntimeConfigs.RuntimeConfigVolumeMount, | ||
| ) | ||
| } | ||
|
|
||
| // Add runtime config volume mount to main container | ||
| if len(podSpec.Containers) > 0 { | ||
| podSpec.Containers[0].VolumeMounts = append( | ||
| podSpec.Containers[0].VolumeMounts, | ||
| commonConfig.RuntimeConfigs.RuntimeConfigVolumeMount, | ||
| ) | ||
| } |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Code review by AI agent
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
LGTM. The Cartesian product fix for NodeSelectorTerms correctly implements AND semantics. MatchLabels and GetDatasetId issues are also resolved. A couple of non-blocking suggestions: (1) consider handling MatchFields in the Cartesian product merge, and (2) add a test case for multiple NodeSelectorTerms on both sides.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Ⅰ. Describe what this PR does
transform Cache Runtime Spec into master/worker/client pods spec
Ⅱ. Does this pull request fix one issue?
part of #5412 and fix #5877
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
unit test
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
RuntimeTieredStorefor worker/client and storage for master(ha) not yet resolved